e2e disk encryption tests by rolandmkunkel · Pull Request #1681 · Azure/ARO-RP

rolandmkunkel · 2021-08-16T16:38:59Z

Which issue this PR addresses:

extends e2e tests to create cluster with disk encryption enabled with customer managed keys.
adds e2e test to confirm following:
All vms has disk encryption set
Storage class is set to provision encrypted disk

Fixes https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/7404076

What this PR does / why we need it:

Helps us testing the e2e disk encryption feature and gives us more confidence to ship it.

github-actions · 2021-08-16T16:42:33Z

Please rebase pull request.

mjudeikis

Few nits but getting close

deploy/generate.go

pkg/deploy/generator/const.go

pkg/deploy/generator/resources_disk_encryption.go

mjudeikis · 2021-09-10T09:45:59Z

pkg/util/cluster/cluster.go

So if this is not available? Maybe try again and not fail?

pkg/util/cluster/cluster.go

m1kola

Few questions/suggestions, but in general looks good so far.

deploy/generate.go

pkg/util/azureclient/keyvault/vaults.go

pkg/util/cluster/cluster.go

pkg/util/instancemetadata/instancemetadata.go

m1kola · 2021-09-10T14:15:16Z

pkg/util/cluster/cluster.go

I think at this point we should probably rename vnetResourceGroup to something more generic as it now hold not only vnet, but key vault as well. I however don't have ideas apart from something stupid like "bring your own resource resource group" :)

Also specificly for this line:

You do c.diskEncryptionSets.Get(...) here but you only use it to retreive diskEncryptionSet.ID. At the same time, few lines below you construct this ID (line 235):

{"/subscriptions/" + c.env.SubscriptionID() + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Compute/diskEncryptionSets/" + diskEncryptionSetName, rbac.RoleReader},

pkg/util/cluster/cluster.go

pkg/util/azureclient/mgmt/compute/disks_addons.go

m1kola · 2021-09-10T14:32:06Z

test/e2e/disk_encryption.go

I think it is fine to check via VMs. But I'm wondering why you can't list disks by resoruce group?

@rolandmkunkel we previously discussed that we need to add a code comment explaining why becasue it is not obvious.

rolandmkunkel · 2021-09-30T10:13:38Z

deploy/cluster-development-predeploy

renamed version of deploy/cluster-predeploy.json

github-actions · 2021-10-04T14:21:52Z

Please rebase pull request.

m1kola

I didn't finish reviewing this PR. Just positng early feedback.

pkg/deploy/generator/const.go

pkg/deploy/generator/resources_disk_encryption.go

m1kola · 2021-10-04T14:21:02Z

test/e2e/disk_encryption.go

@rolandmkunkel we previously discussed that we need to add a code comment explaining why becasue it is not obvious.

m1kola · 2021-10-04T14:23:30Z

test/e2e/disk_encryption.go

I guess this is how we check that PVCs with default storage class provision encrypted disks. Is it right? If so, then we should add a comment. Or even better add two separate By("...") (we might end up with two for loops for the same vms because of that, but that's fine IMO).

Anyway - I think we need some form of clarification here as it is not obvious.

m1kola · 2021-10-04T14:48:33Z

pkg/util/keyvault/keyvault.go

I'm looking at GetSecret and it looks like we are simplifying the interface even further by ommiting keyVersion.

Suggested change

func (m *manager) GetKey(ctx context.Context, keyName string, keyVersion string) (azkeyvault.KeyBundle, error) {

return m.kv.GetKey(ctx, m.keyvaultURI, keyName, keyVersion)

}

func (m *manager) GetKey(ctx context.Context, keyName string) (azkeyvault.KeyBundle, error) {

return m.kv.GetKey(ctx, m.keyvaultURI, keyName, "")

}

This way can probably get rid of LatestKeyVersion constant.

pkg/deploy/generator/resources_disk_encryption.go

m1kola

Still need to review pkg/util/cluster/cluster.go.

m1kola · 2021-10-05T15:58:16Z

pkg/util/azureclient/keyvault/keys.go

I think we no longer use this. Everything is in BaseClient now.

github-actions · 2021-10-08T16:37:01Z

Please rebase pull request.

m1kola

I have a lot of questions.

Also I think this PR now has too many things in the scope:

Updating shared environemt
Dealing with deployments in a non-shared resoruce group.
Dealing with setting up e2e clusters
E2Es tests on their own

I think part of this is my fault: I didn't realise it is earlier. Let's talk tomorrow on what to do with this.

m1kola · 2021-10-06T16:11:57Z

pkg/util/cluster/cluster.go

Is it going to stay? If so, we might want to give it better message and use infof.

m1kola · 2021-10-12T13:31:38Z

pkg/util/cluster/cluster.go

+		res, err := serviceKeyvault.GetKey(ctx, keyName)
+		if err == nil && res.Key != nil {
+			c.log.Infoln("using existing key")
+			shouldCreateKey = false
+		} else {
+			err = serviceKeyvault.RecoverDeletedKey(ctx, keyName)
+			if err != nil {
+				c.log.Infoln("could not recover any existing key")
+			} else {
+				c.log.Infoln("recovered key")
+				shouldCreateKey = false
+			}
+		}


What is the reason why we should reuse keys? Can we create a new key each time for both CI and dev runs?

I would like to get rid of this logic, if possible.

m1kola · 2021-10-12T13:49:06Z

pkg/deploy/generator/resources_dev.go

+		{
+			ObjectID: to.StringPtr("[parameters('azureServicePrincipalId')]"),
+			Permissions: &mgmtkeyvault.Permissions{
+				Keys: &[]mgmtkeyvault.KeyPermissions{mgmtkeyvault.KeyPermissionsCreate, mgmtkeyvault.KeyPermissionsGet, mgmtkeyvault.KeyPermissionsDelete, mgmtkeyvault.KeyPermissionsRecover},


Nit: on formatting.

Suggested change

Keys: &[]mgmtkeyvault.KeyPermissions{mgmtkeyvault.KeyPermissionsCreate, mgmtkeyvault.KeyPermissionsGet, mgmtkeyvault.KeyPermissionsDelete, mgmtkeyvault.KeyPermissionsRecover},

Keys: &[]mgmtkeyvault.KeyPermissions{

mgmtkeyvault.KeyPermissionsCreate,

mgmtkeyvault.KeyPermissionsGet,

mgmtkeyvault.KeyPermissionsDelete,

mgmtkeyvault.KeyPermissionsRecover,

},

m1kola · 2021-10-12T13:52:34Z

test/e2e/disk_encryption.go

+)
+
+var _ = Describe("Encryption at host should be enabled", func() {
+	BeforeEach(skipIfNotInDevelopmentEnv)


m1kola · 2021-10-12T13:56:48Z

test/e2e/disk_encryption.go

+		for _, vm := range vms {
+			vm.StorageProfile.DataDisks = &[]mgmtcompute.DataDisk{{
+				Lun:          to.Int32Ptr(0),
+				CreateOption: mgmtcompute.DiskCreateOptionTypesEmpty,
+				DiskSizeGB:   to.Int32Ptr(1),
+				ManagedDisk: &mgmtcompute.ManagedDiskParameters{
+					DiskEncryptionSet: &mgmtcompute.DiskEncryptionSetParameters{ID: to.StringPtr(diskEncryptionSetId)},
+				},
+			}}
+			err = clients.VirtualMachines.CreateOrUpdateAndWait(ctx, clusterResourceGroup, *vm.Name, vm)
+			Expect(err).NotTo(HaveOccurred())
+		}


Why are we creating VM here?

m1kola · 2021-10-12T15:45:12Z

pkg/deploy/generator/resources_disk_encryption.go

+		},
+	})
+
+	vaultResource.APIVersion = azureclient.APIVersion("Microsoft.KeyVault")


Why not have it in g.keyVault(...)?

Same question for devDiskEncryptionKeyvault() in pkg/deploy/generator/resources_dev.go.

Also take a look at virtualNetwork: we accept condition and dependsOn as params. Should we do something similar for g.keyVault(...)?

ARO-RP/pkg/deploy/generator/resources.go

Line 92 in af54d36

func (g *generator) virtualNetwork(name, addressPrefix string, subnets *[]mgmtnetwork.Subnet, condition interface{}, dependsOn []string) *arm.Resource {

m1kola · 2021-10-12T15:46:04Z

pkg/deploy/generator/resources_disk_encryption.go

+	})
+
+	vaultResource.APIVersion = azureclient.APIVersion("Microsoft.KeyVault")
+	vaultResource.Condition = to.StringPtr("[equals(parameters('shouldCreateKeyVault'), 'true')]")


Can't we just use [parameters('ci')] as a condition?

m1kola · 2021-10-12T15:59:57Z

pkg/util/cluster/cluster.go

+		}
+	}
+
+	diskEncryptionSetName := clusterName + "-des"


Should we have a constant for "-des"?

m1kola · 2021-10-12T16:02:55Z

pkg/deploy/generator/resources_disk_encryption.go

+				{
+					TenantID: &tenantUUIDHack,
+					ObjectID: to.StringPtr("[parameters('rpServicePrincipalId')]"),
+					Permissions: &mgmtkeyvault.Permissions{
+						Keys: &[]mgmtkeyvault.KeyPermissions{
+							mgmtkeyvault.KeyPermissions(mgmtkeyvault.Create),
+							mgmtkeyvault.KeyPermissions(mgmtkeyvault.Delete),
+						},
+					},
+				},


Giving these permissions to RP service principal doesn't sound right. RP SP is for RP resources, not cluster/customer resoruces.

m1kola · 2021-10-12T16:09:05Z

test/e2e/disk_encryption.go

+			Expect(osDisk.Encryption.Type).To(Equal(mgmtcompute.EncryptionAtRestWithCustomerKey))
+		}
+
+		By("checking the encryption property on each data disk of each VM")


This should be about validating that default storrage class creates disks with disk encryption enabled.

See "How to test that default PV is encrypted" in for manual steps: #1627

But I think this PR is too big already: it is very hard to follow. Let's deal wtih it once we merge this.

rolandmkunkel added the work-in-progress label Aug 16, 2021

rolandmkunkel requested a review from m1kola August 16, 2021 16:38

rolandmkunkel requested review from jewzaam and mjudeikis as code owners August 16, 2021 16:39

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch from 57560c9 to 32a46fa Compare August 16, 2021 16:42

github-actions bot added the needs-rebase branch needs a rebase label Aug 16, 2021

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch from 32a46fa to 85d81d1 Compare September 9, 2021 10:13

m1kola mentioned this pull request Sep 10, 2021

ARO Fips Mode Installation #1732

Merged

mjudeikis suggested changes Sep 10, 2021

View reviewed changes

m1kola reviewed Sep 10, 2021

View reviewed changes

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch 2 times, most recently from ae46a5e to a46840b Compare September 23, 2021 08:36

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch from a46840b to 0847e8a Compare September 30, 2021 10:13

github-actions bot removed the needs-rebase branch needs a rebase label Sep 30, 2021

rolandmkunkel commented Sep 30, 2021

View reviewed changes

deploy/cluster-development-predeploy Outdated

Copy link

Contributor Author

rolandmkunkel Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed version of deploy/cluster-predeploy.json

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch 7 times, most recently from b42758a to 792e4a8 Compare October 1, 2021 14:45

rolandmkunkel added ready-for-review and removed work-in-progress labels Oct 4, 2021

rolandmkunkel changed the title ~~WIP e2e disk encryption tests~~ e2e disk encryption tests Oct 4, 2021

github-actions bot added the needs-rebase branch needs a rebase label Oct 4, 2021

m1kola reviewed Oct 4, 2021

View reviewed changes

m1kola reviewed Oct 5, 2021

View reviewed changes

pkg/util/azureclient/keyvault/keys.go Outdated

Copy link

Contributor

m1kola Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we no longer use this. Everything is in BaseClient now.

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch from 792e4a8 to 32261e4 Compare October 7, 2021 11:32

rolandmkunkel requested review from bennerv, hawkowl, mwoodson and rogbas as code owners October 7, 2021 11:32

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch from 32261e4 to 04eb293 Compare October 7, 2021 12:22

github-actions bot removed the needs-rebase branch needs a rebase label Oct 7, 2021

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch 2 times, most recently from ba8eca9 to e91acfc Compare October 7, 2021 13:27

disk encryption e2e tests

6a907bf

rolandmkunkel force-pushed the disc-encryption-e2e-tests branch from e91acfc to 6a907bf Compare October 8, 2021 11:04

github-actions bot added the needs-rebase branch needs a rebase label Oct 8, 2021

m1kola mentioned this pull request Oct 12, 2021

e2e flake - admin API list resources #1783

Merged

m1kola suggested changes Oct 12, 2021

View reviewed changes

m1kola mentioned this pull request Oct 12, 2021

Add DiskEncryptionSet to geneva action ResourcesList #1775

Merged

rolandmkunkel closed this Oct 29, 2021

-func (m *manager) GetKey(ctx context.Context, keyName string, keyVersion string) (azkeyvault.KeyBundle, error) {
-	return m.kv.GetKey(ctx, m.keyvaultURI, keyName, keyVersion)
-}
+func (m *manager) GetKey(ctx context.Context, keyName string) (azkeyvault.KeyBundle, error) {
+	return m.kv.GetKey(ctx, m.keyvaultURI, keyName, "")
+}

Conversation

rolandmkunkel commented Aug 16, 2021 • edited by m1kola Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue this PR addresses:

What this PR does / why we need it:

Uh oh!

github-actions bot commented Aug 16, 2021

Uh oh!

mjudeikis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m1kola left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 4, 2021

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 8, 2021

Uh oh!

m1kola left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rolandmkunkel commented Aug 16, 2021 •

edited by m1kola

Loading

m1kola left a comment •

edited

Loading

m1kola left a comment •

edited

Loading